Skip to content

Integrate the new heap implementation from InfiniTime #97

Merged
NeroBurner merged 5 commits into
mainfrom
heap-unification
May 18, 2023
Merged

Integrate the new heap implementation from InfiniTime #97
NeroBurner merged 5 commits into
mainfrom
heap-unification

Conversation

@JF002

@JF002 JF002 commented Apr 9, 2023

Copy link
Copy Markdown
Contributor

Integration of this PR from InfiniTime.

Since FreeRTOS.h, portmacro_cmsis.h and task.h are now built in C (by lv_mem.c), I had to change some includes and declarations to make them compatible with a C compiler.

Integrating the new memory management from InfiniTime in InfiniSim is not easy because InfiniSim does not include the whole FreeRTOS. Which means that, for example, pvPortMalloc() and vPortFree() are not accessible from InfiniSim. As a first step, I provided custom implementations for pvPortMalloc(), vPortFree() which are based on ... malloc(). These function keep track of the memory that is currently allocated so that xPortGetFreeHeapSize(), xPortGetMinimumEverFreeHeapSize() return something.

Not that this implementation do not keep track of all the memory allocations done in InfiniTime. It can only "see" those done via pvPortMalloc(). It means that the available memory displayed by InfiniSim will probably be very optimistic.

@NeroBurner NeroBurner force-pushed the heap-unification branch 2 times, most recently from 8fa1597 to 7ccff91 Compare May 1, 2023 20:25
Comment thread sim/FreeRTOS.cpp Outdated
}

namespace {
std::map<void *, size_t> allocatedMemory;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we actually need an ordered map? I think an unordered_map would do just fine and has better runtime for search of O(1) in the best case

see: https://stackoverflow.com/a/13799886

Suggested change
std::map<void *, size_t> allocatedMemory;
std::unordered_map<void *, size_t> allocatedMemory;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, indeed, we don't need the map to be ordered!

Comment thread sim/FreeRTOS.cpp Outdated
Comment on lines +23 to +27
size_t currentSize = 0;
std::for_each(allocatedMemory.begin(), allocatedMemory.end(), [&currentSize](const std::pair<void*, size_t>& item){
currentSize += item.second;
});

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice use of <algorithm>, but we can be even more explicit using std::accumulate from <numeric>

Suggested change
size_t currentSize = 0;
std::for_each(allocatedMemory.begin(), allocatedMemory.end(), [&currentSize](const std::pair<void*, size_t>& item){
currentSize += item.second;
});
const size_t currentSize = std::accumulate(
allocatedMemory.begin(), allocatedMemory.end(), 0,
[](const size_t lhs, const std::pair<void*, size_t>& item){
return lhs + item.second;
});

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, it's even better with std::accumulate!

Comment thread sim/task.h
void *instance = nullptr;
};
void *instance;
}TaskHandle_t;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: space before name

Suggested change
}TaskHandle_t;
} TaskHandle_t;

Comment thread sim/task.h
//StackType_t *pxStackBase; /* Points to the lowest address of the task's stack area. */
uint16_t usStackHighWaterMark; /* The minimum amount of stack space that has remained for the task since the task was created. The closer this value is to zero the closer the task has come to overflowing its stack. */
};
}TaskStatus_t;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: space before name

Suggested change
}TaskStatus_t;
} TaskStatus_t;

Comment thread main.cpp Outdated
Comment on lines +853 to +885
auto minimumEverFreeHeap = xPortGetMinimumEverFreeHeapSize();
if (currentFreeHeap != lastFreeHeapSize) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can move the minimum value getter into the if block to only get that value when we really need it

Suggested change
auto minimumEverFreeHeap = xPortGetMinimumEverFreeHeapSize();
if (currentFreeHeap != lastFreeHeapSize) {
if (currentFreeHeap != lastFreeHeapSize) {
auto minimumEverFreeHeap = xPortGetMinimumEverFreeHeapSize();

Comment thread sim/task.h
struct TaskHandle_t {
void *thread_handle = nullptr;
typedef struct TaskHandle_t {
void *thread_handle;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we still default initialize in C code? I actually don't know

Suggested change
void *thread_handle;
void *thread_handle = 0;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mhm I don't think we can do that in C, but this file should be compiled by the C++ compiler so it should work, I guess (and it builds).

@NeroBurner

NeroBurner commented May 2, 2023

Copy link
Copy Markdown
Collaborator

for me the used memory is always 0. Even tough the custom alloc defines are set the pvPortMalloc function is never used in the simulator (at least for me)

#define LV_MEM_CUSTOM_INCLUDE <FreeRTOS.h>   /*Header for the dynamic memory function*/
#define LV_MEM_CUSTOM_ALLOC   pvPortMalloc       /*Wrapper to malloc*/
#define LV_MEM_CUSTOM_FREE    vPortFree         /*Wrapper to free*/

otherwise this PR would be fine to merge as it still works with the current main branch without the memory allocation changes

edit: nevermind, I just needed to properly recompile. Now changing memory is showing

JF002 and others added 5 commits May 3, 2023 22:39
…InfiniTime#1709).

Since FreeRTOS.h, portmacro_cmsis.h and task.h are now built in C (by lv_mem.c), I had to change some includes and declarations to make them compatible with a C compiler.

Integrating the new memory management from InfiniTime in InfiniSim is not easy because InfiniSim does not include the whole FreeRTOS. Which means that, for example, pvPortMalloc() and vPortFree() are not accessible from InfiniSim.
As a first step, I provided custom implementations for pvPortMalloc(), vPortFree() which are based on ... malloc(). These function keep track of the memory that is currently allocated so that xPortGetFreeHeapSize(), xPortGetMinimumEverFreeHeapSize() return something.

Not that this implementation do not keep track of all the memory allocations done in InfiniTime. It can only "see" those done via pvPortMalloc(). It means that the available memory displayed by InfiniSim will probably be very optimistic.
@JF002

JF002 commented May 18, 2023

Copy link
Copy Markdown
Contributor Author

@NeroBurner I agree with all your comments, and thanks for applying the changes in the code! I'll merge the branch in InfiniTime soon so we can merge this one in InfiniSim :)

@NeroBurner NeroBurner merged commit e6dcf3f into main May 18, 2023
@NeroBurner NeroBurner deleted the heap-unification branch May 18, 2023 18:32
@NeroBurner

Copy link
Copy Markdown
Collaborator

you're welcome. Always nice to see a project being used and improved 🥰

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants